Skip to content

perf(queue): back off re-evaluating a PR whose verdict never changes - #10204

Merged
JSONbored merged 2 commits into
mainfrom
perf/verdict-stability-backoff
Jul 31, 2026
Merged

perf(queue): back off re-evaluating a PR whose verdict never changes#10204
JSONbored merged 2 commits into
mainfrom
perf/verdict-stability-backoff

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Closes #10184

metagraphed#8886 was evaluated 56 times on one unchanged commit in 47 minutes — ~1.2/min — producing the identical hold | missing_linked_issue every time. It is CONFLICTING so it cannot merge, held so it does not close, and its linked issue was closed by a merged rival (#10168), so the hold never clears. A state it cannot leave, with nothing throttling re-entry.

Four such PRs produced 66% of all decision records in a two-hour window — the same window that exhausted the installation's GitHub REST quota and stalled 66 queue jobs behind deferred_by: rate_limit.

Why existing coalescing can't help

#10127 collapses simultaneous events. This is a steady drip of legitimately distinct deliveries (CI completions, label writes, sibling activity) arriving long after any window. The missing control is a different kind — not "collapse events that arrive together" but "stop asking a question whose answer has not changed."

Design

Sameness is the decision fields (action, reason_code, hold_cause), deliberately not record_digest: #8886 has 56 distinct digests for 56 identical verdicts because the digest commits to per-evaluation data. Using it would make every repeat look novel — which is how this went unnoticed.

  • Record at the persistDecisionRecord call site — the single ledger write every verdict passes through, so no path bypasses it (the same reasoning that function uses for its own re-evaluation check).
  • Skip after the actuation-lock claim and before the refresh: the check is one cache read on a pass that already owns the PR, and a backed-off pass spends nothing. Composes with perf(queue): claim the actuation lock before the refresh it exists to prevent #10181.
  • Returns rather than throws — not contention, no work to retry, published state already correct. Uses false, this function's existing "did not re-review" signal. No caller branches on it, so it cannot drive a retry loop. Releases the lock first.
  • State keyed on the head SHA, so a new commit resets structurally rather than by a remembered rule.

HOLDS ONLY — the flaw that wiring exposed

The first version backed off any repeated verdict. The force-fresh-rebase test (#9497/#2552) caught it: three deliberate identical passes spend the 24h update-branch cap, and my backoff swallowed the third.

"Same verdict" is not "nothing happened." A pass can take real actions — update-branch, cap accounting — while its verdict is unchanged; throttling that suppresses progress, not waste. A hold is the one action meaning "the gate declined to act", so repeating it genuinely produces nothing. Every other action keeps today's behaviour exactly, and this still covers the motivating case precisely (#8886 is 56 identical holds).

This is the whole argument for not shipping the module unwired — 21 passing unit tests could not have found it.

Safety

Capped delay (stuck PRs still revisited, just not 1.2×/min) · fails OPEN on missing/malformed/absent state or a throwing cache · engages only after 3 repeats so an ordinary webhook/sweep race is unaffected.

Verification

Full suite 26,444 passed; the 2 failures are load-sensitive selfhost-ai subprocess tests unrelated to this change — 240 pass in isolation. typecheck, dead-exports, dead-source-files clean.

Mutation-tested, each caught: remove the cap (liveness — PR never revisited) · fail closed on missing state · don't reset on a changed verdict · drop the calls-equivalent hold restriction.

Mutation testing also deleted code: an exponent clamp that looked like defence-in-depth but that no test could distinguish, because Math.min(Infinity, cap) is already the cap.

…ver changes

metagraphed#8886 was evaluated 56 times on ONE unchanged commit in 47 minutes --
about 1.2 per minute -- producing the identical `hold | missing_linked_issue`
every time. It is CONFLICTING so it cannot merge, held so it does not close, and
its linked issue was closed by a merged rival (#10168), so the hold never
clears. Four such PRs produced 66% of all decision records in a two-hour window,
and that window exhausted the installation's GitHub REST quota.

The webhook coalescer (#10127) cannot help: this is not a burst but a steady
drip of legitimately distinct deliveries arriving long after any window. The
missing control is a different kind -- not "collapse events that arrive
together" but "stop asking a question whose answer has not changed".

Sameness is the DECISION fields (action, reason_code, hold_cause) and
deliberately NOT record_digest: #8886 has 56 distinct digests for its 56
identical verdicts, because the digest commits to per-evaluation data. Using it
would make every repeat look novel, which is how this went unnoticed.

Safety properties, each pinned by a test: the delay is capped so a stuck PR is
still revisited; state is keyed on the head SHA so a new commit resets
structurally rather than by a rule someone must remember; a changed verdict
resets the count; and every uncertain case -- no state, malformed state, no
cache, a throwing cache -- fails OPEN. A backoff that engaged on missing
information would silently stop reviewing PRs, which is worse than the churn.

Mutation testing removed a clamp on the exponent that looked like defence in
depth: `2 ** 1000` is Infinity and `Math.min(Infinity, cap)` is the cap, so no
test could distinguish its presence. The cap is the only thing bounding this and
it is tested directly; a second guard nobody can verify is a claim, not a
safeguard.

The module is registered in STAGED_AHEAD_OF_CONSUMERS. Wiring the skip is
deliberately a separate change: it suppresses re-evaluation in processors.ts's
hottest path, and getting it wrong stops reviewing PRs rather than merely
wasting work. That belongs in a focused diff with its own review, not appended
to this one.

Refs #10184
Completes the backoff by connecting it, rather than leaving the module staged
ahead of its consumers. Wiring it immediately found a design flaw its own 21
tests could not: "same verdict" is NOT "nothing happened".

RECORD at the persistDecisionRecord call site -- the single ledger write every
verdict passes through, so no path can bypass it, the same reasoning that
function uses for its own reevaluation check. Keyed on the head SHA, so a new
commit starts clean structurally rather than by a rule someone must remember.

SKIP after the actuation-lock claim and before the refresh: the check is then
one cache read on a pass that already owns the PR, and a backed-off pass spends
nothing. It composes with #10174's reorder. It RETURNS rather than throws --
this is not contention, there is no work to retry, and the published state is
already correct -- using `false`, the same "did not re-review" signal this
function's other early bail uses. No caller branches on the result, so it cannot
drive a retry loop. The lock is released explicitly first; wiring is what
surfaced that leak.

HOLDS ONLY, and this is the flaw wiring exposed. The force-fresh-rebase test
(#9497/#2552) runs three deliberate identical passes to spend the 24h
update-branch cap, and the first version swallowed the third: those passes take
real actions -- update-branch, cap accounting -- while producing an unchanged
verdict. Throttling them suppresses progress, not waste. A `hold` is the one
action meaning "the gate declined to act", so repeating it genuinely produces
nothing; every other action keeps today's behaviour exactly. That also covers
the motivating case precisely -- #8886 is 56 identical HOLDS on one commit.

Removed from STAGED_AHEAD_OF_CONSUMERS: it has consumers now.

Closes #10184
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Important

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏳ LoopOver is waiting…

LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored self-assigned this Jul 31, 2026
@JSONbored
JSONbored merged commit 38a09c9 into main Jul 31, 2026
4 checks passed
@JSONbored
JSONbored deleted the perf/verdict-stability-backoff branch July 31, 2026 12:34
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.95238% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.34%. Comparing base (3884485) to head (e5be077).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/queue/processors.ts 46.15% 6 Missing and 1 partial ⚠️
src/review/verdict-stability.ts 96.55% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10204      +/-   ##
==========================================
- Coverage   92.21%   91.34%   -0.88%     
==========================================
  Files         934      935       +1     
  Lines      114178   114220      +42     
  Branches    27593    27609      +16     
==========================================
- Hits       105294   104334     -960     
- Misses       7582     8777    +1195     
+ Partials     1302     1109     -193     
Flag Coverage Δ
backend 94.12% <80.95%> (-1.56%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/review/verdict-stability.ts 96.55% <96.55%> (ø)
src/queue/processors.ts 94.32% <46.15%> (-0.19%) ⬇️

... and 3 files with indirect coverage changes

JSONbored added a commit that referenced this pull request Jul 31, 2026
…r an operator force (#10229)

#10204 placed the skip at the entry of the publish-and-maintain pass, past two
things it must not run past.

Readiness fires options.onReachedReadiness -- which charges regatePullRequest's
bounded repair budget -- and then consumes the ONE-SHOT panel-retrigger marker
(#7626). A guard sitting after both meant a backed-off pass had already spent a
user's "Re-run LoopOver review" click with nothing left to re-trigger it, and had
charged a repair attempt for work it never did. The guard now sits between the
readiness gate and onReachedReadiness.

It must not move EARLIER than readiness either: readiness legitimately defers a
pass, and the screenshot-table recapture chain (#10061) depends on those
deferrals to bound its retry budget -- a pre-readiness guard truncated it from 5
attempts to 3, caught by that test.

options.force is now honoured. An operator's manual re-gate passes force: true
and was being silently suppressed; a poll tick (previewPollAttempt) likewise.
Backoff exists to stop the machine re-asking itself a settled question and must
never suppress a pass a human asked for.

The guard is extracted so both publish-and-maintain sites can share it, and its
!headSha half is documented as a TSC-enforced early-out rather than a safety
guard -- mutation testing confirms no runtime test can distinguish its absence,
and an unverifiable guard is a claim, not a safeguard (verdict-stability.ts's own
removed exponent clamp made the same point).

#10204 shipped this wiring with NO test; test/unit/verdict-stability-wire.test.ts
is the first, and pins each defect above as a regression.

The webhook path is deliberately still unguarded. Adding it there truncates
#10061's recapture budget, and the fix is not another exemption but moving the
skip to the verdict-derivation choke point the record half already uses --
scoped in #10227.

Closes #10222
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf: no backoff for a PR whose verdict never changes — 56 identical evaluations of one commit in 47 minutes

1 participant